New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
helm: add optional monitoring RBAC to operator chart #9388
Conversation
079782a
to
5cc752d
Compare
@sathieu would you care to test whether this PR's changes (with |
5cc752d
to
7bd17ff
Compare
@@ -4,7 +4,6 @@ These should be scoped to the namespace where the CephCluster is located. | |||
*/}} | |||
|
|||
{{- define "library.cluster.monitoring.roles" -}} | |||
# --- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this was ever here....
Documentation/ceph-upgrade.md
Outdated
@@ -319,6 +319,9 @@ step to upgrade the Prometheus RBAC resources as well. | |||
kubectl apply -f deploy/examples/monitoring/rbac.yaml | |||
``` | |||
|
|||
Or, if you use only the `rook-ceph` operator Helm chart, you should also add `monitoring.enabled` to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also need to mention that setting this is not needed if they set the same thing in the cluster helm chart? It's in both places now, right?
Or, if you use only the `rook-ceph` operator Helm chart, you should also add `monitoring.enabled` to | |
Or, if you use only the `rook-ceph` operator Helm chart, you could enable monitoring by setting `monitoring.enabled`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is needed in some fashion. The monitoring resources were previously at least partially applied by the Helm chart by default. I think it is actually a step users should do if they previously used Helm and monitoring. Maybe there is a better way I could word this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the same setting is named in both charts, what if we just say:
Or, if you are installing with helm, set `monitoring.enabled` to `true` in either the `rook-ceph` or `rook-ceph-cluster` chart.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh. It doesn't matter if you enable it in both charts. There is an if-then in the rook-ceph-cluster
chart that omits that RBAC if the cluster namespace is the same as the operator namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if the operator and cluster are in the same namespace you have to specify it in the operator chart, and if in different namespaces you have to specify in the cluster chart? Seems like we would prefer it to be set in the cluster chart if they are using it, so maybe we should remove that condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not quite right.
To firstly clarify: nothing has changed about the behavior I mentioned in this PR. Currently, the rook-ceph
chart is deployed to its namespace. The rook-ceph-cluster
chart is deployed to a namespace and also takes an operatorNamespace
value that tells the chart where the operator is located. If the rook-ceph-cluster
chart is being deployed into the same namespace as operatorNamespace
, it will assume the rook-ceph
chart has already deployed the resources it needs (including the monitoring RBAC) and will not deploy them a second time. If we want to change that behavior, it is orthogonal to this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thanks for the clarification, that would be a much bigger design change than just for this monitoring, so let's not touch it. So what wording do you recommend here? Seems like it should mention that if the cluster is created in the same namespace as the operator, the monitoring needs to be enabled with that setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can revise this wording, but I think it's generally safe to say that if you have monitoring enabled, set monitoring.enabled
in the Helm chart at this point in the upgrade.
Otherwise, we have to get deep into different cases of do or don't set the value:
- what if the user uses the
rook-ceph-cluster
chart inside the operator namespace (do set this value) - what if the user uses the
rook-ceph-cluster
chart outside the operator namespace (don't set here)- but what if there is also a
rook-ceph-cluster
chart inside the operator namespace? (do still set here)
- but what if there is also a
- what if the user creates cluster resources themselves in the operator namespace (do still set here)
- what if the user creates cluster resources themselves outside the operator namespace (don't set here)
- but what if there is also a cluster inside the operator namespace (do still set here)
4 out of 6 cases have the user setting this enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, let's keep it simple. :)
Documentation/helm-operator.md
Outdated
@@ -148,6 +148,7 @@ The following tables lists the configurable parameters of the rook-operator char | |||
| `admissionController.tolerations` | Array of tolerations in YAML format which will be added to admission controller deployment. | <none> | | |||
| `admissionController.nodeAffinity` | The node labels for affinity of the admission controller deployment (***) | <none> | | |||
| `allowMultipleFilesystems` | **(experimental in Octopus (v15))** Allows multiple filesystems to be deployed to a Ceph cluster. | `false` | | |||
| `monitoring.enabled` | Create necessary RBAC rules for Rook to integrate with prometheus monitoring. Requires Prometheus to be pre-installed. | `false` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe rename this to monitoring.rbacEnable
? It's only about RBAC currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would tend to agree, but this is the same naming used for the rook-ceph-cluster
chart. I think keeping consitency might be preferred over a better name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It creates the same exact resources as the cluster chart for monitoring.enabeld? If so, makes sense to me that it would be named the same
Documentation/ceph-upgrade.md
Outdated
@@ -319,6 +319,9 @@ step to upgrade the Prometheus RBAC resources as well. | |||
kubectl apply -f deploy/examples/monitoring/rbac.yaml | |||
``` | |||
|
|||
Or, if you use only the `rook-ceph` operator Helm chart, you should also add `monitoring.enabled` to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, let's keep it simple. :)
7bd17ff
to
7725144
Compare
If you use the `rook-ceph` operator Helm chart, you should also add `monitoring.enabled` to | ||
your Helm values with two caveats: | ||
- this is unnecessary if you deploy monitoring RBAC from `deploy/examples/monitoring/rbac.yaml` | ||
- this is unnecessary if you use `rook-ceph-cluster` charts exclusively outside of the `rook-ceph` | ||
operator namespace. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you feel about this updated/clarified wording @travisn ?
An older version of the Helm chart always installed RBAC permissions for enabling monitoring. In an effort to reduce the privileges Rook uses by default, they were removed. We need to still include the monitoring RBAC optionally since the change could break some users. Co-authored-by: Mathieu Parent <mathieu.parent@insee.fr> Co-authored-by: Blaine Gardner <blaine.gardner@redhat.com> Signed-off-by: Blaine Gardner <blaine.gardner@redhat.com>
7725144
to
e3be7a3
Compare
helm: add optional monitoring RBAC to operator chart (backport #9388)
An older version of the Helm chart always installed RBAC permissions for
enabling monitoring. In an effort to reduce the privileges Rook uses by
default, they were removed. We need to still include the monitoring RBAC
optionally since the change could break some users.
Co-authored-by: Mathieu Parent mathieu.parent@insee.fr
Co-authored-by: Blaine Gardner blaine.gardner@redhat.com
Signed-off-by: Blaine Gardner blaine.gardner@redhat.com
Description of your changes:
Which issue is resolved by this Pull Request:
Resolves #9398 (with the caveat that values
monitoring.enabled
should be set totrue
)Checklist:
make codegen
) has been run to update object specifications, if necessary.